Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

db: allow requesting a default if key not found #1673

Merged
merged 2 commits into from
Nov 22, 2019

Conversation

half-duplex
Copy link
Member

I have a few places in my plugins where I follow a get_x_value with if value is None: value = .... This adds an optional default argument to make that easier.

@half-duplex
Copy link
Member Author

I ran flake8 after the functional changes, but not after adding the tests =|

@half-duplex
Copy link
Member Author

Depending on merge order, affects #1621

@dgw
Copy link
Member

dgw commented Aug 20, 2019

I'm lukewarm on this one. Increasing the complexity of our getter functions and adding an optional argument for the few cases where a falsy value is meaningfully different from the default seems like overkill. Those cases are free to use the explicit pattern you described, or the slightly more concise:

v = get_x_value(*args)
value = v if v is not None else 'default'

Plugins that need to retrieve the same value with the same default in several places are better off defining an internal getter-wrapper, anyway, regardless of whether its body contains one (with this patch) or two (without this patch) lines of statements. (Potential bias note: A few of my IRC games use this model, so I can handle all the default fallbacks and other value validation in a single place, before returning a value to the code that needs it. So I'm used to doing things this way anyway.)

My potential bias aside, I believe that in the vast majority of cases, value = get_x_value(*args) or 'default' works just fine (but admittedly, I have not conducted an empirical study of existing plugins in the wild). The code here looks fine, though, so it's probably merge-ready if I change my mind (and/or am convinced to).

@half-duplex
Copy link
Member Author

The tradeoff is four lines and an optional argument in Sopel, or boilerplate in many plugins using the DB to handle defaulting. I can't imagine it's an uncommon need since every one of my nontrivial plugins would benefit, so I would rather the mild increased complexity be in the library than in every implementation.

Poking quickly through the builtin modules:

  • pronouns.py needs more complex logic
  • safety.py needs more complex logic
  • seen.py needs more complex logic, I think
  • reddit.py directly uses None as falsy
  • clock.py could drop 3-4 lines
  • adminchannel.py could drop 2 lines

So of the 11 plugins I've looked at that use the DB, 7 of them would benefit (and obviously none would be harmed). If you would like me to make those changes to clock and adminchannel, this PR's non-test changes could reduce the project's line count =p

@dgw
Copy link
Member

dgw commented Aug 25, 2019

@half-duplex Would you add commits updating the relevant plugins to use the new argument? I think I'm coming around on this, and it'd be good to have everything updated (in core) at once.

Also, by the project's general FIFO merge order, #1621 will be merged first. So plan to rebase and add this same functionality to plugin values. (You might or might not have luck rebasing directly onto that PR's branch, if you want to get the rebase done ASAP. GitHub can be unpredictable regarding how it handles duplicated commits when PRs get merged.)

Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on this. However, I like the added tests, that's nice, and the change itself isn't big, so code-wise, I'm ok with that. 👍

@dgw
Copy link
Member

dgw commented Aug 30, 2019

#1621 is merged. Update when ready. 😸

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this escape being assigned a milestone?

If I ever find myself in a time machine, I might go back and ask myself.

@dgw dgw added this to the 7.0.0 milestone Nov 21, 2019
@dgw dgw merged commit d00e08a into sopel-irc:master Nov 22, 2019
@half-duplex half-duplex deleted the db-default branch November 24, 2019 02:17
dgw added a commit that referenced this pull request May 25, 2021
Missing since #1673, first released in 7.0.0-rc1. Whoops.
dgw added a commit that referenced this pull request May 25, 2021
Missing since #1673, first released in 7.0.0-rc1. Whoops.

Doc changes only, so [skip CI].
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants